GlobalDeclareCheck: new check for declare -A without -g in global scope#779
Closed
AngryLoki wants to merge 1 commit intopkgcore:masterfrom
Closed
GlobalDeclareCheck: new check for declare -A without -g in global scope#779AngryLoki wants to merge 1 commit intopkgcore:masterfrom
AngryLoki wants to merge 1 commit intopkgcore:masterfrom
Conversation
Author
|
132 warnings in ::gentoo Results for ::gentoo
|
arthurzam
reviewed
Apr 15, 2026
Any use of declare is local scope unless a -g is passed. Missing -g can cause issues when the ebuild is sourced inside a function (as non-portage package managers may do). One example is pkgcore: when it processes ebuilds with declare -A without -g, it usually fails in configure phase (good outcome) or worse, it silently skips some files in install phase. The new check is added as a warning since Portage is not affected. Closes: pkgcore#628 Signed-off-by: Sv. Lockal <lockalsash@gmail.com>
Author
|
135 warnings in ::gentoo Details
|
ferringb
reviewed
Apr 21, 2026
Contributor
ferringb
left a comment
ferringb
left a comment
There was a problem hiding this comment.
For my own knowledge- how'd you flush this out? What workflow were you upto where this came up?
It's esoteric and should be checked for, I'm just curious if you caught this due to pkgcheck reasons, or were playing around w/ parts of pkgcore.
| return f"line {self.lineno}: call to 'declare -A' without '-g' in global scope: {self.line}" | ||
|
|
||
|
|
||
| class GlobalDeclareCheck(Check): |
Contributor
There was a problem hiding this comment.
In the future, please slot classes. Pkgcheck is lax on this, but the bulk (or base) of pkgcore ecosystem leans towards slotting as a defensive measure against typos in pathways that lack test coverage.
Not a block, just a FYI for future code.
| - StabilizationGroupsCheck: check for invalid and non-existant stabilization | ||
| groups (Arthur Zamarin) | ||
|
|
||
| - GlobalDeclareCheck: detect ``declare -A`` without ``-g`` in global scope |
Contributor
There was a problem hiding this comment.
Thanks for adding that; walking the history to update the news is a minor pain the ass. :)
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.decl_query = bash.query("(declaration_command) @decl") |
Contributor
There was a problem hiding this comment.
I realize you were pointed at "add it to init", but this is effectively a constant- thus should be class level. If you have further refactoring, can you incorporate that?
If not, I'll sort it post merge if I remember.
| if child.type == "word": | ||
| flag = pkg.node_str(child) | ||
| if flag.startswith("-"): | ||
| if "A" in flag: |
Contributor
There was a problem hiding this comment.
stylistic nit/food for thought: has_A |= "A "in flag
Not a requirement, I'm just pointing it out.
Contributor
|
Also- being pkgcore does everything within functions due to the daemon, is |
Author
|
Draftified for now. At first I thought it only applies to associative arrays, but it also affects normal arrays. var1="test"
declare -- var2="test"
declare -A var3=([key1]="value1")
declare -a var4=(1 2 3)
src_unpack() {
declare -p var1 var2 var3 var4
die
}emerge output: pmerge / paludis output: Maybe a better idea is to warn about any global |
Contributor
@ulm from the PMS angle, what's your thoughts here? I do not want to pull tricks out of my ass, but I do think people treat ebuilds as effectively "directly executed bash scripts"- the old portage mechanism of straight out sourcing the ebuild as global script execution still exists, based on the code I just checked. For anything modern, that's just not acceptable in the ebuild env implementation- we use functions for valid reasons, however I agree it's not great making devs deal w/ the quirks of bash for function vs global. Said another way; I don't want to change pkgcore because this is literally what portage was supposed to do, and you know well the perf gains from our approach. |
Contributor
|
Just dumping some thoughts here: the pkgcore ebuild internals, specifically the env saving machinery, is 'aware' enough it could promote this sort of thing to global on it's own. It'd require significant unit test coverage, but basically we'd do this for actual phase execution:
It's doable, it's just not any form of awesome. |
Member
|
I think every implementation but Portage runs into this (i.e. I think pkgcraft also hit it, but it was a long while ago now). |
Author
Pointed directly by @negril in #guru IRC. Then successfully forgot about this, did the same mistake again, then decided that it might be better to add a check before I forget in a third time. From the linked task I know that it is possible to work around this. Practically all times I missed As I said, I can try to count of warnings for all global |
Contributor
|
@AngryLoki I opened pkgcore/pkgcore#470 to rework the internal EBD env mechanisms to remove this problem. Please comment there, but what you've clarified, I don't think ebuild devs should have to force global. They write ebuilds as if it's a bash script, pkgcore is violating devex/intent on this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Any use of
declareis local scope unless a-gis passed. Missing-gcan cause issues when the ebuild is sourced inside a function (as non-portage package managers may do).One example is pkgcore: when it processes ebuilds with
declare -Awithout-g, it usually fails in configure phase (good outcome) or worse, it silently skips some files in install phase.The new check is added as a warning since Portage is not affected.
Closes: #628